Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scale.alpha_continuous and Scale.alpha_discrete #1252

Closed
wants to merge 1 commit into from

Conversation

Mattriks
Copy link
Member

@Mattriks Mattriks commented Feb 16, 2019

  • I've updated the documentation to reflect these changes
  • I've added an entry to NEWS.md
  • I've added and/or updated the unit tests
  • I've run the regression tests
  • I've built the docs and confirmed these changes don't cause new errors

This PR:

So it is no longer necessary to use Theme(lowlight_color=) for opacity. But for example Theme(lowlight_color=c->"gray") is still useful for changing the line color of Geom.ribbon and Geom.polygon independently of the color aesthetic.

Examples:

palettef = Scale.lab_gradient("darkgreen","orange", "blue")
p1 = plot(x=1:10, y=rand(10), color=[1:10;], Geom.point,  
    Scale.color_continuous(colormap=palettef, minvalue=0, maxvalue=10),
    Guide.title("Scale.color_continuous, Theme(alphas=[0.5])"),
    Theme(alphas=[0.5], continuous_highlight_color=identity, point_size=2mm)
)
p2 = plot(x=1:10, y=rand(10), alpha=[1:10;], Geom.point,  
    Scale.alpha_continuous(minvalue=0, maxvalue=10),
    Guide.title("Scale.alpha_continuous, Theme(default_color=\"blue\")"),
    Theme(default_color="blue", discrete_highlight_color=c->"gray", point_size=2mm)
)
hstack(p1, p2)

alpha_continuous

D = DataFrame(x=1:6, y=[0.39, 0.26, 0.31, 0.43, 0.1, 0.78], Shape=repeat(["a","b","c"], outer=2))
coord = Coord.cartesian(xmin=0, xmax=7, ymin=0, ymax=1.0)
p1 = plot(D, x=:x, y=:y, color=:x,  coord,
    Scale.color_discrete, Geom.point, Geom.hair,
    Guide.title("Scale.color_discrete, Theme(alphas=[0.5])"),
    Theme(alphas=[0.5], discrete_highlight_color=identity, point_size=2mm)
)
p2 = plot(D, x=:x, y=:y, alpha=:x, shape=:Shape, coord,
    Scale.alpha_discrete, Geom.point, Geom.hair,
    Guide.title("Scale.alpha_discrete, Theme(default_color=\"green\")"),
    Theme(default_color="green",   discrete_highlight_color=c->"gray", point_size=2mm,
       alphas=[0.0,0.2,0.4,0.6,0.8,1.0])
)
hstack(p1,p2)

alpha_discrete

@codecov-io
Copy link

codecov-io commented Feb 16, 2019

Codecov Report

Merging #1252 into master will decrease coverage by 0.24%.
The diff coverage is 95.65%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1252      +/-   ##
=========================================
- Coverage   85.74%   85.5%   -0.25%     
=========================================
  Files          35      35              
  Lines        4119    4139      +20     
=========================================
+ Hits         3532    3539       +7     
- Misses        587     600      +13
Impacted Files Coverage Δ
src/data.jl 0% <ø> (ø) ⬆️
src/aesthetics.jl 77.96% <ø> (ø) ⬆️
src/Gadfly.jl 74.81% <ø> (-0.49%) ⬇️
src/theme.jl 65.51% <0%> (-4.66%) ⬇️
src/geom/ribbon.jl 100% <100%> (ø) ⬆️
src/geom/point.jl 100% <100%> (ø) ⬆️
src/scale.jl 94.56% <100%> (+0.49%) ⬆️
src/geom/polygon.jl 92.5% <100%> (+1.07%) ⬆️
src/guide/keys.jl 91.13% <100%> (ø) ⬆️
src/guide.jl 84.39% <100%> (ø) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82922a3...2ab689f. Read the comment docs.

@bjarthur
Copy link
Member

looks great! any chance you have time to add Guide.alphakey ? and thanks for tackling all these old issues!!

@Mattriks
Copy link
Member Author

I ran the regression testing, and see that the discrete colorkey swatch size is smaller with this PR, because the swatches are now stroked with the default_discrete_highlight_color which is RGB(1,1,1) (white). Previously the key swatches were unstroked so the points were larger: I've mimicked this in the first key below by setting the Theme(discrete_highlight_color=c->nothing).

Because the key swatches are now stroked in this PR (to enhance the use of the alpha aesthetic), I see 3 options: make the default_discrete_highlight_color = nothing (Option 1 = first key below), or make the default_discrete_highlight_color = identity function (Option 2 = third key). A third option would be to the leave the default_discrete_highlight_color = white. What would you prefer for the default_discrete_highlight_color?

discrete_colorkey
Note that no matter what the default_discrete_highlight_color function is, you can change it via Theme(discrete_highlight_color=).

@Mattriks
Copy link
Member Author

Here's a good example, where there are lots of bunched points:

D = dataset("datasets","faithful")
D[:g] = D[:Eruptions].>3.0
ylab = Guide.ylabel(nothing)
p1 = plot(D, x=:Eruptions, y=:Waiting, color=:g, Geom.point, ylab,
    Theme(discrete_highlight_color=c->nothing, alphas=[0.4], key_position=:inside)  )
p2 = plot(D, x=:Eruptions, y=:Waiting, color=:g, Geom.point, ylab,
    Theme(discrete_highlight_color=c->"white", key_position=:inside) )
p3 = plot(D, x=:Eruptions, y=:Waiting, color=:g, Geom.point, ylab,
    Theme(discrete_highlight_color=identity, alphas=[0.4], key_position=:inside)  )
hstack(p1, p2, p3)

discrete_highlight_color3

@bjarthur
Copy link
Member

so in the regression tests, the points in the plot are the same as before, it's just that the swatches are different? if so, and it's now the case that the swatches are identical to the points in the plot (except for the mismatch in shapes), whereas before they weren't, then i'd consider this a bug fix, not a breaking change.

IIUC, any changes to default_discrete_highlight_color would change the appearance of the points in the plot, and that i'd consider breaking change.

@Mattriks
Copy link
Member Author

Yes the points in the plot are the same as before, the swatches (in discrete colorkeys) are smaller because they get stroked with white (the default_discrete_highlight color). So you seem to be voting for option 3. That's fine by me, if users want non-white stroked points and swatches, I've given examples in the docs about how to change it (same as in the plots in my OP above).

I'll do Guide.alphakey in a future PR. I think it would be good to look at Guide.linekey (#1042) next.

@Mattriks
Copy link
Member Author

I noticed that if you click on the graphics in my 2 previous posts, the white stroke around the key swatches in the middle plot can clearly be seen (I'm using Chrome). If there are no other comments, this PR is ready to merge.

@@ -177,14 +177,14 @@ Gadfly.helpscreen_visible = function(event) {
helpscreen_visible(this.plotroot());
};
var helpscreen_visible = function(root) {
root.select(".helpscreen").animate({opacity: 1.0}, 250);
root.select(".helpscreen").animate({"fill-opacity": 1.0}, 250);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this appears to be a duplicate of c62ef81, and is how the code already looks on master. how is it then that this is showing up as a change in the diff?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At some point I rebased this branch on master, because I started it before c62ef81. What does github think the diff is relative to?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you click on the "commits" tab above it says "bjarthur authored and Mattriks committed 12 days ago". i think the rebase messed things up. am worried master will get messed up if i merge this. can you please see if you can fix it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how did you do your rebase? my workflow is as follows:

git checkout master
git pull
git checkout -b newPR
git add
git commit
git checkout master
git pull
git checkout newPR
git rebase master

the key here is that master is rebase onto newPR, not vice versa, which is what i think you might have done.

@@ -213,7 +214,8 @@ hstack(p3, p4)
| `shape` | `shape_discrete` | `shapekey` |
| `size` | `size_discrete` | sizekey (tbd) |
| `linestyle` | `linestyle_discrete` | linekey (tbd) |
| `group` | `group_discrete` | |
| `alpha` | `alpha_discrete` | alphakey (tbd) |
| `group` | `group_discrete` | |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not sure the tables of Scales are appropriate in the Tutorial. they are all enumerated in the Library. we don't enumerate all Guides or Geoms. and listing them here disrupts the flow of teaching by example. i might submit a PR to smooth things out.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tables were introduced in a previous PR (#1245): http://gadflyjl.org/dev/tutorial/. The relationship between aesthetic, scales and guides is a generally not well understood, and hence these tables need to be in the tutorial.

@Mattriks
Copy link
Member Author

Mattriks commented Mar 3, 2019

Closed in favor of #1257

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

svgjs, hstack and opacity opacity in the color scale
3 participants